batch synced site creation #7309

diracdeltas · 2017-02-18T01:37:08Z

Test Plan:

in about:preferences#sync, choose the 'i have an existing sync code' option. enter in the code words that Alex posted in #testers (starts with 'forsaken')
open about:bookmarks and wait for 6000+ bookmarks to be synced. the browser should not freeze or use excessive memory during the sync.

diracdeltas · 2017-02-18T02:23:08Z

looks like syncing bookmarks tests are failing so i need to fix those

bridiver · 2017-02-21T15:07:38Z

I think this is part of a more general issue with appstate performance and we want to batch all updates in the dispatcher to solve some other perf issues. Is there any reason this code can't run in the background page?

diracdeltas · 2017-02-21T19:43:10Z

Is there any reason this code can't run in the background page?

it would be some effort to replace all the calls to AppStore.getState() and appActions with IPC between background page and browser. which part of this is most helpful to move to the background page?

bridiver · 2017-02-21T19:46:13Z

@diracdeltas the loop that calls the appActions. That part seems like it should be pretty straightforward to move and then I can handle the batching with the appDispatcher stuff I'm working on

ayumi

i know we're discussing moving some of this code around, but i reviewed for the concept of batching.

for ~1k bookmarks it reduces memory consumption which is sweet!

however syncing bookmarks in folders doesn't work, and they end up folderless– comments inline 🎷

ayumi · 2017-02-22T22:26:00Z

js/state/syncUtil.js

+      const parentFolderObjectId = siteProps.parentFolderObjectId
+      if (parentFolderObjectId && parentFolderObjectId.length > 0) {
+        siteProps.parentFolderId =
+          getFolderIdByObjectId(new Immutable.List(parentFolderObjectId))


for initial sync, if we apply all bookmarks and folders at once, this will always return null; so folders don't sync.

we could fix this by first applying folders and folderless bookmarks and folders (to ensure correct ordering); then applying foldered bookmarks. see below for more info

@ayumi i think there is a general race condition here even if we apply stuff in the right hierarchy order. see #7293 (comment)

ayumi · 2017-02-22T22:30:42Z

js/state/syncUtil.js

@@ -194,10 +232,22 @@ module.exports.applySyncRecord = (record) => {
 */
 module.exports.applySyncRecords = (records) => {
  if (!records || records.length === 0) { return }
+  const siteRecords = []
+  const otherRecords = []
+  records.forEach((record) => {


bookmark folders:

instead of applying all bookmarks, i think we should first apply all folderless bookmarks and folders, then apply foldered bookmarks. otherwise we won't be able to resolve bookmark.parentFolderObjectId -> parentFolderId, so bookmarks will end up folderless.

bonus points to recursively apply (top level folders/items, then second level, etc) to better ensure folders ids are available when we need them.

diracdeltas · 2017-02-23T03:56:59Z

@bridiver i tried moving this code to the background script; it didn't beach-ball the browser but it did make the UI non-responsive indefinitely. i think the batching is necessary to solve the perf issue.

bridiver · 2017-02-23T03:58:38Z

Batching is almost ready

…

On Feb 22, 2017, at 8:57 PM, yan ***@***.***> wrote: @bridiver i tried moving this code to the background script; it didn't beach-ball the browser but it did make the UI non-responsive indefinitely. i think the batching is necessary to solve the perf issue. — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub, or mute the thread.

bridiver · 2017-02-23T23:23:17Z

js/state/syncUtil.js

+        appActions.removeSite(siteDetail, tag, true)
+      }
+    })
+  }, 2000)


if this has to happen after creation a timeout doesn't seem very safe, or is the timeout just an extra buffer? If so I don't think it's really necessary with the dispatcher changes

diracdeltas · 2017-02-24T08:01:48Z

js/state/syncUtil.js

+  let existingObjectData
+
+  if (record.action !== writeActions.CREATE) {
+    // XXX: Is this necessary for CREATEs also?


omitting the existingObject search for CREATES reduces the sync lag significantly, from a minute or so to a couple seconds.

affirming that existing objects aren't needed – it would have been resolved upstream during get_existing_objects -> resolved_sync_records

diracdeltas · 2017-02-24T08:02:23Z

updated PR so now it fixes the bookmark folder issue too

ayumi · 2017-02-24T19:47:13Z

it worked really fast for the forsaken ... profile!

then i tested with my special user data which is mildly corrupted (for example some bookmarks include nonexistent parentFolderIds), and it crashed.

you can replicate this by editing session-store-1 to change a bookmark's parentFolderId to something bogus, then starting Brave and doing the initial sync upload. it doesn't crash, but it prevents records from being uploaded.

in sync.js should syncUtil.createSiteData(siteJS) be also sending appState? https://github.com/brave/browser-laptop/blob/feature/batch-sync/app/sync.js#L207

TypeError: Cannot read property 'getIn' of undefined
    at findOrCreateFolderObjectId (/Users/ayu/repos/github.com/brave/browser-laptop/js/state/syncUtil.js:366:26)
    at Object.module.exports.createSiteData (/Users/ayu/repos/github.com/brave/browser-laptop/js/state/syncUtil.js:403:31)
    at syncBookmark (/Users/ayu/repos/github.com/brave/browser-laptop/app/sync.js:207:29)
    at /Users/ayu/repos/github.com/brave/browser-laptop/node_modules/immutable/dist/immutable.js:2701:43
    at List.__iterate (/Users/ayu/repos/github.com/brave/browser-laptop/node_modules/immutable/dist/immutable.js:2208:13)
    at OrderedMap.__iterate (/Users/ayu/repos/github.com/brave/browser-laptop/node_modules/immutable/dist/immutable.js:2700:25)
    at OrderedMap.forEach (/Users/ayu/repos/github.com/brave/browser-laptop/node_modules/immutable/dist/immutable.js:4383:19)
    at module.exports.onSyncReady (/Users/ayu/repos/github.com/brave/browser-laptop/app/sync.js:218:6)
    at emitOne (events.js:96:13)
    at EventEmitter.emit (events.js:188:7)

diracdeltas · 2017-02-24T20:20:24Z

@ayumi does that issue exist without this PR?

ayumi · 2017-02-24T20:25:48Z

@diracdeltas no, it works on master as of d9234b5

diracdeltas · 2017-02-24T20:38:04Z

@ayumi i think its fixed now

ayumi · 2017-02-24T20:51:51Z

@diracdeltas sweet it works now!

bridiver · 2017-02-24T21:00:18Z

js/stores/appStore.js

@@ -485,6 +487,32 @@ const handleAppAction = (action) => {
    case appConstants.APP_DATA_URL_COPIED:
      nativeImage.copyDataURL(action.dataURL, action.html, action.text)
      break
+    case appConstants.APP_APPLY_SITE_RECORDS:


can we leave a comment here about refactoring this into a single action with APP_ADD_SITE? When we do that we should probably pull it out of appStore anyway and create a siteReducer

or maybe the comment should go on the action. Either way is fine, just want to capture it in the code

alexwykoff · 2017-02-24T21:06:03Z

I ran bookmark tests on 7a18689 and found 4 failures

71 passing (3m)
4 failing

about:bookmarks page content "before all" hook:
WaitUntilTimeoutError: Promise was rejected with the following reason: Error: unknown error: Cannot read property 'appStoreRenderer' of undefined
at execute() - brave.js:407:19
Syncing bookmarks create bookmark in folder:
Promise was rejected with the following reason: timeout
Error
Syncing bookmarks update bookmark, moving it into the folder:
Promise was rejected with the following reason: Error: An element could not be located on the page using the given search parameters.
Error
Syncing bookmarks from an existing profile "before all" hook:
WaitUntilTimeoutError: Promise was rejected with the following reason: timeout
at windowHandles() - brave.js:208:32

diracdeltas · 2017-02-24T21:08:31Z

@alexwykoff could you try on 9a2b535 ?

alexwykoff · 2017-02-24T21:24:45Z

Sure!

Here's the latest:
3 failing

about:bookmarks page content "before all" hook:
WaitUntilTimeoutError: Promise was rejected with the following reason: Error: unknown error: Cannot read property 'appStoreRenderer' of undefined
at execute() - brave.js:407:19
bookmark tests bookmarks bookmark pdf check location:
WaitUntilTimeoutError: Promise was rejected with the following reason: timeout
at execute() - brave.js:413:19
navigationBar tests auto open bookmarks toolbar for the first bookmark should remain hidden if user has bookmarks but has toolbar hidden:
WaitUntilTimeoutError: element (#navigator .removeBookmarkButton) still not existing after 10000ms
at isExisting("#navigator .removeBookmarkButton") - waitForExist.js:37:22

I don't think these are on you, they look like flakey timeouts.

bridiver · 2017-02-24T21:26:27Z

@alexwykoff I think some of those are fixed in #7353
the number of intermittent test failures is way down with those changes

@ayumi

fix #7308 fix #7293 fix #7307 Auditors: @ayumi @alexwykoff Test Plan: 1. in about:preferences#sync, choose the 'i have an existing sync code' option. enter in the code words that Alex posted in #testers (starts with 'forsaken') 2. wait for 6000+ bookmarks to appear in the bookmarks toolbar. the browser should not freeze or use excessive memory during the sync. for me it takes about 5 seconds. 3. open about:bookmarks. you should see a bunch of folders with bookmarks, including subfolders.

ayumi · 2017-02-24T20:55:18Z

js/state/syncUtil.js

+  let existingObjectData
+
+  if (record.action !== writeActions.CREATE) {
+    // XXX: Is this necessary for CREATEs also?


affirming that existing objects aren't needed – it would have been resolved upstream during get_existing_objects -> resolved_sync_records

diracdeltas · 2017-02-24T23:10:03Z

syncing tests all fixed now!

srirambv · 2017-03-01T13:59:53Z

@diracdeltas There is a momentary spike in the CPU usage when bookmarks are sync'd. Brave process's CPU usage goes upto 30% and drops down. It took ~2mins to sync the bookmarks on my machine with a clean profile.

bridiver · 2017-03-01T16:06:01Z

@srirambv when you say it took ~2 mins, what exactly are you referring to? Did the spike last 2 min? Was it 2 min until they appeared on another device?

srirambv · 2017-03-01T16:35:49Z

@bridiver The total time to sync was around 2 mins for the bookmarks to shown up on the second device. The CPU spike was momentary around 10 secs (could hear the fan loud and clear), happened initially when the sync code was added and then right before the bookmarks showed up on the second device.

diracdeltas · 2017-03-01T18:50:07Z

sync polls every 1 minute, so on average i would expect ~30s for a sync to occur

bridiver · 2017-03-01T21:40:19Z

If the poll time is 1 minute the average should be closer to 1 minute to sync across devices. 2 minutes would be a theoretical worst case assuming the timers are accurate (which they aren't) and the sync process is instantaneous (which it isn't)

…

On Mar 1, 2017, at 11:50 AM, yan ***@***.***> wrote: sync polls every 1 minute, so on average i would expect ~30s for a sync to occur — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub, or mute the thread.

bridiver · 2017-03-01T21:41:04Z

Future versions of this will improve latency with something like SQS

…

On Mar 1, 2017, at 11:50 AM, yan ***@***.***> wrote: sync polls every 1 minute, so on average i would expect ~30s for a sync to occur — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub, or mute the thread.

diracdeltas · 2017-03-01T22:51:00Z

If the poll time is 1 minute the average should be closer to 1 minute to sync across devices.

it sends ~immediately after the state change but the receiver polls every 1min

diracdeltas added the feature/sync label Feb 18, 2017

diracdeltas added this to the 0.13.5 milestone Feb 18, 2017

diracdeltas requested review from ayumi and alexwykoff February 18, 2017 01:37

diracdeltas added the PR/work-in-progress ⚒ label Feb 18, 2017

diracdeltas mentioned this pull request Feb 18, 2017

Bookmark folder tree structure is not properly synced between devices #7293

Closed

ayumi suggested changes Feb 22, 2017

View reviewed changes

diracdeltas force-pushed the feature/batch-sync branch from 4690c01 to 597483b Compare February 23, 2017 22:33

bridiver reviewed Feb 23, 2017

View reviewed changes

diracdeltas force-pushed the feature/batch-sync branch from 597483b to 00d3186 Compare February 24, 2017 08:00

diracdeltas added PR/needs-QA-attention ☕ and removed PR/work-in-progress ⚒ labels Feb 24, 2017

diracdeltas commented Feb 24, 2017

View reviewed changes

bridiver reviewed Feb 24, 2017

View reviewed changes

diracdeltas force-pushed the feature/batch-sync branch from 9a2b535 to fcb4fc5 Compare February 24, 2017 21:26

ayumi approved these changes Feb 24, 2017

View reviewed changes

Update newtab and history pages after sync records are applied

fa18022

diracdeltas merged commit db6cf28 into master Feb 24, 2017

diracdeltas deleted the feature/batch-sync branch February 24, 2017 23:10

luixxiul removed the PR/needs-QA-attention ☕ label Feb 25, 2017

This was referenced Feb 25, 2017

apply sync records in batches #7308

Closed

[Intermittent Issue] Bookmarks added before initial sync are not always copied over to pyramid 1 #7305

Closed

srirambv mentioned this pull request Mar 1, 2017

Windows x64 test plan for 0.13.5 preview 3 #7424

Closed

batch synced site creation #7309

batch synced site creation #7309

Conversation

diracdeltas commented Feb 18, 2017 • edited Loading

diracdeltas commented Feb 18, 2017

bridiver commented Feb 21, 2017

diracdeltas commented Feb 21, 2017

bridiver commented Feb 21, 2017

ayumi left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

diracdeltas commented Feb 23, 2017

bridiver commented Feb 23, 2017 via email

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

diracdeltas commented Feb 24, 2017

ayumi commented Feb 24, 2017 • edited Loading

diracdeltas commented Feb 24, 2017

ayumi commented Feb 24, 2017

diracdeltas commented Feb 24, 2017

ayumi commented Feb 24, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

alexwykoff commented Feb 24, 2017

diracdeltas commented Feb 24, 2017

alexwykoff commented Feb 24, 2017

bridiver commented Feb 24, 2017

Choose a reason for hiding this comment

diracdeltas commented Feb 24, 2017

srirambv commented Mar 1, 2017 • edited Loading

bridiver commented Mar 1, 2017

srirambv commented Mar 1, 2017

diracdeltas commented Mar 1, 2017

bridiver commented Mar 1, 2017 via email

bridiver commented Mar 1, 2017 via email

diracdeltas commented Mar 1, 2017

diracdeltas commented Feb 18, 2017 •

edited

Loading

ayumi commented Feb 24, 2017 •

edited

Loading

srirambv commented Mar 1, 2017 •

edited

Loading